src/Domain/Common/ExternalImageService.php (1)

150-166: Minor inconsistency: file_get_contents fallback doesn't respect $verifySsl.

The cURL path now respects the verifySsl config, but the file_get_contents fallback still uses PHP's default SSL behavior. If someone sets verify_ssl=false expecting to bypass verification, this fallback path would still verify (or vice versa depending on PHP config).

Not a blocker, but worth considering for consistency if both paths should behave the same way.

🔧 Optional: Add SSL context to file_get_contents

 private function downloadUsingFileGetContent(string $filename): string
 {
     $remoteURLContext = stream_context_create([
         'http' => [
             'method' => 'GET',
             'timeout' => $this->externalImageTimeout,
             'max_redirects' => '10',
-        ]
+        ],
+        'ssl' => [
+            'verify_peer' => $this->verifySsl,
+            'verify_peer_name' => $this->verifySsl,
+        ],
     ]);

     $cacheFileContent = file_get_contents($filename, false, $remoteURLContext);

src/Domain/Messaging/Service/ForwardDeliveryService.php (1)

39-57: Consider using constants or an enum for status values.

The magic strings 'sent' and 'failed' are used here and likely referenced elsewhere. Extracting them to constants (or a PHP 8.1+ enum) would improve type safety and make refactoring easier down the road.

💡 Example using class constants

 class ForwardDeliveryService
 {
+    public const STATUS_SENT = 'sent';
+    public const STATUS_FAILED = 'failed';
+
     public function __construct(
         // ...
     }

     public function markSent(Message $campaign, Subscriber $subscriber, string $friendEmail): void
     {
         $this->messageForwardManager->create(
             subscriber: $subscriber,
             campaign: $campaign,
             friendEmail: $friendEmail,
-            status: 'sent'
+            status: self::STATUS_SENT
         );
     }

     public function markFailed(Message $campaign, Subscriber $subscriber, string $friendEmail): void
     {
         $this->messageForwardManager->create(
             subscriber: $subscriber,
             campaign: $campaign,
             friendEmail: $friendEmail,
-            status: 'failed'
+            status: self::STATUS_FAILED
         );
     }
 }

src/Domain/Messaging/Service/ForwardingGuard.php (1)

47-52: Consider extracting status string to a constant.

The 'sent' status string appears here and likely in other places. A shared constant (or enum) would prevent typos and make the status values discoverable.

Optional improvement

+    private const STATUS_SENT = 'sent';
+
     public function hasAlreadyBeenSent(string $friendEmail, Message $campaign): bool
     {
         $existing = $this->forwardRepository->findByEmailAndMessage($friendEmail, $campaign->getId());

-        return $existing !== null && $existing->getStatus() === 'sent';
+        return $existing !== null && $existing->getStatus() === self::STATUS_SENT;
     }

src/Domain/Messaging/Service/ForwardContentService.php (1)

40-40: TODO noted - link handling for forwards.

Worth tracking this as an issue so it doesn't get lost. Forwarded messages might need special treatment for click tracking or unsubscribe links.

Would you like me to open an issue to track the link handling investigation for forwarded messages?

tests/Unit/Domain/Messaging/Service/ForwardDeliveryServiceTest.php (1)

67-113: Consider extracting service instantiation to reduce duplication.

The ForwardDeliveryService instantiation is repeated identically in all four test methods. You could DRY this up with a helper method or by creating the service in setUp(). Not a big deal for a small test class, but it'd make future maintenance a bit easier.

♻️ Optional: Extract to setUp or helper

 class ForwardDeliveryServiceTest extends TestCase
 {
     private MailerInterface&MockObject $mailer;
     private UserMessageForwardManager&MockObject $forwardManager;
+    private ForwardDeliveryService $service;

     protected function setUp(): void
     {
         $this->mailer = $this->createMock(MailerInterface::class);
         $this->forwardManager = $this->createMock(UserMessageForwardManager::class);
+        $this->service = new ForwardDeliveryService(
+            mailer: $this->mailer,
+            messageForwardManager: $this->forwardManager,
+            bounceEmail: 'bounce@example.test',
+        );
     }

     public function testSendUsesBounceEnvelopeAndRecipient(): void
     {
-        $service = new ForwardDeliveryService(
-            mailer: $this->mailer,
-            messageForwardManager: $this->forwardManager,
-            bounceEmail: 'bounce@example.test',
-        );
-
         $email = (new Email())->to('friend@example.test');
         // ... rest of test using $this->service

src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php (1)

174-187: The * 2 multiplier on line 181 is unclear.

The condition ($this->forwardPersonalNoteSize * 2) > strlen($note) feels like magic. Why multiply by 2? A comment referencing the ticket (0011996) exists, but the logic itself could use a brief inline explanation for future maintainers.

Also, strlen($note) counts bytes, not characters. For multi-byte (UTF-8) notes, you might want mb_strlen($note, 'UTF-8') if the intent is to limit by character count rather than byte length.

💡 Suggested clarification

         $escapedNote = htmlspecialchars($note, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8');
-        //0011996: forward to friend - personal message
-        if (($this->forwardPersonalNoteSize * 2) > strlen($note)) {
+        // 0011996: forward to friend - personal message
+        // Allow notes up to twice the configured max size (legacy behavior).
+        $maxAllowedLength = $this->forwardPersonalNoteSize * 2;
+        if ($maxAllowedLength > mb_strlen($note, 'UTF-8')) {
             $htmlMessage = nl2br($escapedNote) . '<br/>' . $htmlMessage;
             $textMessage = $note . "\n" . $textMessage;
         }

tests/Unit/Domain/Messaging/Service/ForwardingGuardTest.php (1)

105-139: Good coverage for hasAlreadyBeenSent scenarios.

Both the true case (existing forward with 'sent' status) and false case (no existing forward) are tested. The mock setup correctly verifies the interaction with findByEmailAndMessage.

One small note: you might also want a test case for when a forward exists but with a non-'sent' status (e.g., 'failed') to ensure that returns false. Currently only the "none exists" case is tested for the false path.

tests/Unit/Domain/Messaging/Service/MessageForwardServiceTest.php (1)

190-194: The logicalOr constraint may not fully validate both formats are used.

The constraint self::logicalOr(OutputFormat::Html, OutputFormat::Text) verifies that each call receives either Html or Text, but doesn't guarantee that both formats are actually used across the two calls. If both calls happened to use OutputFormat::Html, this test would still pass.

If you want stricter verification that both formats are used exactly once, consider using withConsecutive (deprecated in PHPUnit 10+) or capturing the arguments for manual assertion.

That said, this is a minor point since the mock setup at lines 182-184 already controls which format is returned for each call, so the test is still meaningful.

tests/Unit/Domain/Messaging/Service/Builder/HttpReceivedStampBuilderTest.php (1)

55-72: Keep this test hermetic—avoid real reverse DNS lookups.
Line 59 relies on gethostbyaddr() behavior for a TEST‑NET IP. That can still hit resolvers and be flaky/slow in CI. Consider stubbing DNS resolution or injecting a resolver so you can deterministically force the “no reverse DNS” path.

💡 Example: stub gethostbyaddr in the builder namespace (test-only)

<?php
declare(strict_types=1);

namespace PhpList\Core\Domain\Messaging\Service\Builder {
    function gethostbyaddr(string $ip): string|false
    {
        return false; // force fallback to IP-only
    }
}

namespace PhpList\Core\Tests\Unit\Domain\Messaging\Service\Builder;

tests/Unit/Domain/Messaging/Service/ForwardingStatsServiceTest.php (1)

82-85: Mock expectation set after invocation won't work as intended.

The expects(self::once()) on line 70 already ensures only one call happens throughout the test, so the test does verify the reset behavior. However, setting expects(self::never()) on line 83 after the invocation on line 80 is misleading - PHPUnit expectations should be configured before the calls occur.

The test passes for the right reasons (the once() constraint), but this code block suggests a verification that isn't actually happening. Consider removing lines 82-85 or restructuring for clarity.

🧹 Suggested cleanup

         $service->incrementFriendsCount($subscriber);
         $service->incrementFriendsCount($subscriber);
         $service->updateFriendsCount($subscriber);

-        // Second update attempt should be a no-op due to cache reset
-        $this->attrManager->expects(self::never())->method('createOrUpdateByName');
+        // Second update is a no-op (cache was reset) - verified by the once() expectation above
         $service->updateFriendsCount($subscriber);
-        self::assertTrue(true);
     }

tests/Unit/Domain/Identity/Service/AdminNotifierTest.php (1)

51-68: Same withConsecutive deprecation note applies here.

Just like in the other test file, withConsecutive() is deprecated in PHPUnit 10+. Consider refactoring to use a callback with invocation tracking if you upgrade.

tests/Unit/Domain/Identity/Service/AdminCopyEmailSenderTest.php (1)

113-119: withConsecutive will break in PHPUnit 10+ (already deprecated since 9.6).

Heads up — withConsecutive() was deprecated in PHPUnit 9.6 and fully removed in PHPUnit 10. The project is currently on 9.5, so it's fine now, but if you upgrade PHPUnit to 10 or later, this will stop working. When you get there, refactor to use a callback with invocation tracking (similar to your existing send method expectations).

No urgency, but worth flagging for a future PHPUnit upgrade.